fix: add threading lock for _processing_models set in partial.py - #2462
fix: add threading lock for _processing_models set in partial.py#2462truecallerabreham wants to merge 3 commits into
Conversation
The module-level _processing_models set is mutated without locking in _process_generic_arg and __class_getitem__. In concurrent async/threaded use, two threads processing different models can corrupt the set. Add a threading.Lock to guard mutations of the shared set. Closes 567-labs#2461
ErenAta16
left a comment
ErenAta16
left a comment
There was a problem hiding this comment.
Two problems here, and the second one is a regression rather than a scoping question.
1. A lock doesn't fix the bug in #2461.
The reported failure isn't set corruption, it's a logical race on shared guard state. Serializing access to a still-global set keeps the sharing intact:
thread A: acquires lock, Address not in set, adds it, releases lock, recurses into Address
thread B: acquires lock, sees Address IS in set -> returns raw unwrapped Address
Thread B still gets the wrong schema. The lock only prevents two threads mutating the set at the same instant, and that was never the observable failure. There's no for x in _processing_models anywhere in this file, only in / .add / .discard, which are individually atomic under the GIL, so RuntimeError: set changed size during iteration was never reachable either. The guard needs to be per-call-stack, not shared-with-a-mutex. That's what #2489 does with a ContextVar.
2. The refactor moves try outside the else and disables the recursion guard.
In the __class_getitem__ hunk:
with _processing_models_lock:
if annotation in _processing_models:
tmp_field.annotation = annotation # guard hit: keep unwrapped
else:
_processing_models.add(annotation)
try:
tmp_field.annotation = Partial[annotation] # runs unconditionally now
finally:
with _processing_models_lock:
_processing_models.discard(annotation) # discards even if this call never addedOriginally the try lived inside the else. Now it runs on both branches, so the guard-hit path immediately overwrites tmp_field.annotation with Partial[annotation] and recurses anyway. Simulating both control flows on a self-referential model:
ORIGINAL -> ok, max recursion depth 1
PR #2462 -> guard failed: runaway recursion
So a TreeNode-style model with children: list["TreeNode"] would recurse until it blows the stack. The finally is also now unbalanced: it discards the annotation even on the path that didn't add it, stomping the outer frame's guard entry.
3. Scope: this also carries the citation.py fix from #2459, which is a separate issue with its own PRs (#2460, #2488, #2492). Worth splitting so each can be judged on its own.
I'd close this in favour of #2489 for the partial.py half and one of the citation PRs for the other. Not a criticism of the instinct, a lock is the obvious first reach for anything labelled "thread-unsafe", it just isn't what this particular bug needs.
|
Superseded by #2495, which isolates the partial-model recursion guard with ContextVar state rather than sharing a process-wide lock. |
3 participants
Summary
Add a
threading.Lock()to guard mutations of the shared_processing_modelsset inpartial.py.Problem
The module-level
_processing_models: set[type]is mutated without locking in_process_generic_arg(lines 272-282) and__class_getitem__(lines 639-648). In concurrent async/threaded use (common with instructor), two threads processing different models can corrupt the set, leading toRuntimeError: set changed size during iterationor missed recursion guards.Fix
Add
import threadingand_processing_models_lock = threading.Lock(). Wrap theadd/discardmutations in both_process_generic_argand__class_getitem__withwith _processing_models_lock:.The lock is held only around the check-and-modify pattern, not around the recursive call inside the
tryblock, to avoid deadlocks.Issue
Closes #2461